Skip to content

posix: implement putmsg() #67711

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 31, 2024
Merged

Conversation

AbhinavMir
Copy link
Contributor

Fixes #66978

Copy link
Member

@ycsin ycsin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there, please have a look at our contribution guidelines and make sure that the commit title & message conform to our standard.

Haven't looked at the details, some obvious issues:

  • files missing copyright notice
  • formatting, can be fixed quite easily in vscode (shift + alt + f, or ctrl + shift + p -> "Format document")
  • comments should be /* comment */ instead of // comment

@ycsin ycsin added the area: POSIX POSIX API Library label Jan 18, 2024
@ycsin
Copy link
Member

ycsin commented Jan 19, 2024

Have a look at how to rebase this branch on top of the main branch (instead of merging the main branch into it) and how to squash commits + force push with git, you may need a few tries to make it right, but that's perfectly normal for a first time contributor.

@ycsin ycsin requested a review from cfriedt January 19, 2024 02:25
@AbhinavMir
Copy link
Contributor Author

Hi, do I need to add the path to stropts.h anywhere to make sure the build is successful? It seems that the test keeps failing with cannot find putmsg()

#include <zephyr/ztest.h>
#include <stropts.h>
#include <errno.h>

ZTEST(stropts, test_putmsg)
{
	int ret = putmsg(0, NULL, NULL, 0);

	zassert_equal(ret, -1, "Expected return value -1, got %d", ret);
	zassert_equal(errno, ENOSYS, "Expected errno ENOSYS, got %d", errno);
}

ZTEST_SUITE(stropts, NULL, NULL, NULL, NULL, NULL);

Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AbhinavMir - remember to add entries in lib/posix/Kconfig and lib/posix/CMakeLists.txt

For this one, maybe CONFIG_POSIX_PUTMSG?

@zephyrbot
Copy link
Collaborator

zephyrbot commented Jan 21, 2024

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@AbhinavMir
Copy link
Contributor Author

Hi @ycsin I got my tests running, I also did a rebase, squash, and force. Does this look right?

@ycsin
Copy link
Member

ycsin commented Jan 22, 2024

Hi @ycsin I got my tests running, I also did a rebase, squash, and force. Does this look right?

It seems like you merged the upstream main branch into your PR branch instead of rebasing your PR branch on top of the upstream main branch, that results in a lot of commits and file changes that are not related to your changes.

This can be done (in your PR branch) if you are using VSCode (after fetching remote branch):
image

As things stand now, it might be easier for you to simply backup your work, hard reset this PR branch to upstream main, then apply your changes on top again.

@AbhinavMir
Copy link
Contributor Author

It seems like you merged the upstream main branch into your PR branch instead of rebasing your PR branch on top of the upstream main branch, that results in a lot of commits and file changes that are not related to your changes.

This makes sense, my bad! @ycsin Would it make more sense to make a new PR? Since this PR will be looked at by people who want to commit to STREAMS and the current conversation might be too long to parse through?

@ycsin
Copy link
Member

ycsin commented Jan 22, 2024

It seems like you merged the upstream main branch into your PR branch instead of rebasing your PR branch on top of the upstream main branch, that results in a lot of commits and file changes that are not related to your changes.

This makes sense, my bad! @ycsin Would it make more sense to make a new PR? Since this PR will be looked at by people who want to commit to STREAMS and the current conversation might be too long to parse through?

People won't be notified if PR is still in draft, I think it should be fine to clean this up within this PR, this happens very often to new contributors.

cfriedt
cfriedt previously approved these changes Jan 29, 2024
@AbhinavMir AbhinavMir marked this pull request as ready for review January 29, 2024 01:14
@AbhinavMir AbhinavMir force-pushed the posix/putmsg branch 2 times, most recently from a69bb3d to 1e6020a Compare January 29, 2024 04:54
@AbhinavMir
Copy link
Contributor Author

@ycsin rebased and squashed, and made sure all tests pass!

ycsin
ycsin previously approved these changes Jan 29, 2024
@ycsin
Copy link
Member

ycsin commented Jan 29, 2024

@AbhinavMir this needs a rebase

Minimal header for stropts

Signed-off-by: Abhinav Srivastava <[email protected]>
Add needed KConfig, CMakeList and Stropts.c

Signed-off-by: Abhinav Srivastava <[email protected]>
@AbhinavMir AbhinavMir dismissed stale reviews from ycsin and cfriedt via f45f225 January 29, 2024 10:10
Add tests, Fix config issues, Re-add Timer

Signed-off-by: Abhinav Srivastava <[email protected]>
@AbhinavMir
Copy link
Contributor Author

@ycsin @cfriedt Just wondering if we could merge and close this PR? I'd love to get started on the other STREAMS APIs. I suppose I could branch off of this, rebase and send in fixes for those, but just wondering for sake of convenience.

@nashif
Copy link
Member

nashif commented Jan 30, 2024

maybe I missing something here, but is the purpose of this PR to just introduce putmsg as a not supported implementation?

int putmsg(int fildes, const struct strbuf *ctlptr, const struct strbuf *dataptr, int flags)
{
	ARG_UNUSED(fildes);
	ARG_UNUSED(ctlptr);
	ARG_UNUSED(dataptr);
	ARG_UNUSED(flags);

	errno = ENOSYS;
	return -1;
}

so just a placeholder?

@cfriedt
Copy link
Member

cfriedt commented Jan 30, 2024

maybe I missing something here, but is the purpose of this PR to just introduce putmsg as a not supported implementation?

That is correct. The symbol needs to exist so that a conformant application can link.

@kartben
Copy link
Collaborator

kartben commented Jan 30, 2024

Looks like docs should be updated? https://docs.zephyrproject.org/latest/services/portability/posix/option_groups/index.html#xopen-streams
I haven't been paying attention but I'm guessing this might be true for other recently added functions too (sched_getscheduler(), sched_getparam(), ...?)

@nashif
Copy link
Member

nashif commented Jan 30, 2024

The symbol needs to exist so that a conformant application can link.

link yes, but it will not work if the implementation just returns an error....

How the documentation will need to change i.e. in doc/services/portability/posix/option_groups/index.rst

would we put a "yes" next to putmsg? I guess not, right?, because it is not implemented.

@ycsin
Copy link
Member

ycsin commented Jan 31, 2024

would we put a "yes" next to putmsg? I guess not, right?, because it is not implemented.

it is 'supported' (in terms of 'can be compiled') but the function isn't implemented (ENOSYS), I agree that we can probably note in the documentation about these unimplemented functions (in another PR) just to be clear

I'm guessing this might be true for other recently added functions too (sched_getscheduler(), sched_getparam(), ...?)

yes you are right

@carlescufi carlescufi merged commit 5248587 into zephyrproject-rtos:main Jan 31, 2024
Copy link

Hi @AbhinavMir!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: POSIX POSIX API Library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

posix: implement putmsg()
7 participants